Skip to content

Dynamically type getters and hassers for parameters #152

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 7 commits into from
May 2, 2021
Merged

Dynamically type getters and hassers for parameters #152

merged 7 commits into from
May 2, 2021

Conversation

@Kocal
Copy link
Contributor

@Kocal Kocal commented May 1, 2021 •
edited
Loading

Hi!
This is a proposal for #127.

I've re-used the same logic for services:

  • read and parse the Symfony container XML dump (it would have been simpler with a PHP dump 😛)
  • create a ParameterMap which contains all parameters

Dynamic return types have been implemented for:

  • ParameterBagInterface#get and ParameterBagInterface#has
  • ContainerInterface#getParameter and ContainerInterface#hasParameter
  • AbstractController#getParameter and Controller#getParameter

However, I'm not able to do the same for AbstractController#getParameter and Controller#getParameter when using $this->getParameter('...') inside a AbstractController and Controller. It always use the return type from AbstractController#getParameter. Is this a limitation from PHPStan or I did something wrong @ondrejmirtes?

Also in Symfony 4.0 (lowest deps), there is no method AbstractController#getParameter() and it make the tests failing. What is the best way to support lowest and highest Symfony version in tests and extension? What about using Symfony 4.4 as minimal requirements and dropping Controller usage? This is the only 4.x supported version right now.
EDIT: Nevermind I fixed in c2d3f79 by using 4.4 as lowest version for symfony/framework-bundle, and manually create the default type if a parameter does not exist.

@Kocal Kocal changed the title feat: create Parameter, ParameterMap, ParameterMapFactory, etc... Dynamically type getters and hassers for parameters May 1, 2021
@Kocal Kocal mentioned this pull request May 1, 2021
Closed
ondrejmirtes
ondrejmirtes requested changes May 2, 2021
View reviewed changes
Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, I love this! This is well thought out and I found only a single minor problem :) I wish that more people like you contributed to PHPStan :) Thank you.

@ondrejmirtes
Copy link
Member

ondrejmirtes commented May 2, 2021

Alright: a586e24

I'm gonna squash this and release it :)

@ondrejmirtes ondrejmirtes merged commit a207db0 into phpstan:master May 2, 2021
@ondrejmirtes
Copy link
Member

ondrejmirtes commented May 2, 2021

Thank you!

@mhujer
Copy link
Contributor

mhujer commented May 2, 2021 •
edited
Loading

@Kocal 👍 👏 Just tested your branch on our project and it found the following:
(all of them are valid reports which need to be fixed in our code - all are related to request parameter)

  • Offset 0 does not exist on array<string>|string|null.
  • Cannot cast array<string>|string|null to int. (at (int) $rawAmount)
  • Parameter #1 $input of method FooBar::fromString() expects string|null, array<string>|string|null given. (when passing a $request->query->all()['amount'])
  • Parameter #1 $parameters of method BarBaz::filterParameters() expects array<string>, array<string, array<string>|string> given when passing $request->query->all()

@ondrejmirtes
Copy link
Member

ondrejmirtes commented May 2, 2021

Enjoy it in phpstan-symfony 0.12.29 :)

@Kocal Kocal deleted the GH-127 branch May 2, 2021 09:35
@Kocal
Copy link
Contributor Author

Kocal commented May 2, 2021

Nice! :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ondrejmirtes ondrejmirtes ondrejmirtes approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Kocal @ondrejmirtes @mhujer